-
Notifications
You must be signed in to change notification settings - Fork 10
feat: add --ignore-failed-ci #441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
cmd/wait-for-github/pr.go
Outdated
| ), | ||
| }, | ||
| &cli.BoolFlag{ | ||
| Name: "stop-on-failed-ci", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be more clear if the flag were named something like continue-on-failed-ci or ignore-failed-ci which would default to false to preserve the existing behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, done at c4389f1
cmd/wait-for-github/pr_test.go
Outdated
| Closed: true, | ||
| CIStatus: github.CIStatusFailed, | ||
| }, | ||
| allowFailedCI: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using a test-only field here, I think it makes more sense to directly set the prConfig field for the test scenario:
| allowFailedCI: true, | |
| continueOnFailedCI: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Commented for one fix to the README doc and a nit (not blocking so feel free to ignore). Also it looks like the PR title is being flagged by the lint check. I think you can just make it something like "feat: Add --ignore-failed-ci". |
dblinkhorn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed the commit before last a binary was added. Once that is removed the rest looks good to go!
Co-authored-by: Doug Blinkhorn <52138617+dblinkhorn@users.noreply.github.com> Signed-off-by: Toni Cárdenas <toni@tcardenas.me>
8a7ffe3 to
38942c4
Compare
@dblinkhorn Wow, well spotted, I'm not sure how that happened. I amended the commit to remove the binary from history at aa6f542. (A squash would've done that too, but just in case.) |
dblinkhorn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀
|
Hey @dblinkhorn, just a reminder about this change. I don't have permissions to merge myself, so, just making sure it doesn't fall through the cracks. |
|
Glad you said something because I didn't even think about that! Thanks! |
|
@tcard It looks like this needs a rebase a conflicts resolved. |
A failed CI doesn't necessarily mean the PR won't get merged. CI could be retried, or it could be non-blocking.
Add a
--ignore-failed-ciflag to skip the CI check, and rely only on the merged or closed status. Defaults to false for backwards-compatibility.